-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start documenting traits internals #958
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Just some nitpicks. Can be merged without a second review.
``HasTraits`` object ``obj``) are analogous to those for attribute retrieval. | ||
The starting point is ``has_traits_setattro`` in the source. | ||
|
||
- First we look for the name ``name`` in ``obj``s instance traits dictionary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: obj
's (missing an apostrophe) ?
The formatting gets a bit upset here (as shown on GitHub and with rst2html). Looks like we need an extra space between obj
and "'s".
Same applies to the bullet point below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded to avoid the awkward possessive. See what you think.
the ``trait_object`` struct in the C source) has three relevant fields: | ||
``getattr``, ``setattr`` and ``post_setattr``. These fields contain C function | ||
pointers, and are invoked during attribute get and set operations, as described | ||
below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check my understanding: I think the "C structure underlying each CTrait
instance" is referring to the object referenced by CTrait.handler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the handler; I meant the trait_object
C struct. I'll attempt to reword to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, I'm referring to these three struct fields:
Lines 217 to 219 in de2eaa2
trait_getattr getattr; /* Get trait value handler */ | |
trait_setattr setattr; /* Set trait value handler */ | |
trait_post_setattr post_setattr; /* Optional post 'setattr' handler */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworded the paragraph, though I think what's really needed is a much fuller description of the mechanics. Any better?
Codecov Report
@@ Coverage Diff @@
## master #958 +/- ##
==========================================
+ Coverage 73.05% 73.16% +0.10%
==========================================
Files 51 51
Lines 6514 6507 -7
Branches 1309 1308 -1
==========================================
+ Hits 4759 4761 +2
+ Misses 1363 1352 -11
- Partials 392 394 +2
Continue to review full report at Codecov.
|
Ready for re-review, but I might find time to expand the attribute get/set stuff a bit more before this goes in, to link the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworded the paragraph, though I think what's really needed is a much fuller description of the mechanics. Any better?
Yes, much clearer! Thank you!
Ready for re-review, but I might find time to expand the attribute get/set stuff a bit more before this goes in, to link the getattr slot in the trait_object struct to what happens in the handler.
I am okay with either (1) merge this first and expand it in a separate PR or (2) expand this before merging. I can also dismiss also my own review for now if it makes things clearer, just let me know.
This PR represents a start on documenting Traits internals. It only covers a very small part of the story so far, but I hope it'll expand over time. We're ultimately aiming for the internals documentation to be coherent and complete. For this PR, it's obviously incomplete but I hope it's not incoherent (and if reviewers find it incoherent, it should be fixed).
It also adds the
_instance_traits
and_class_traits
introspection methods to the API documentation, so that they can be referred to.